feat: multi-url support #59
Conversation
The authentication tests (AuthBasicSuccess, AuthBasicFailed, AuthTokenSuccess, AuthTokenFailed) were failing due to protocol and port mismatches. The dummy HTTP server was starting on HttpPort without proper HTTPS support, but tests were trying to connect using HTTPS protocol. Changes: - Changed all auth tests from https:// to http:// protocol - Changed HttpsPort references to HttpPort for consistent port matching - Removed tls_verify=unsafe_off parameters (not needed with plain HTTP) - Added missing .StopAsync() calls for proper server cleanup All 10 authentication tests now pass successfully. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughScopes dummy server state per port; adds multi-address HTTP sender with per-address HttpClient caching and rotation; extends buffer/sender APIs for Guid/char (and nullable); adds Docker-based integration tests and multi-URL tests; and bumps CI/projects to .NET 10. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Sender as Sender Client
participant HttpSender as HttpSender
participant AddrProv as AddressProvider
participant Cache as ClientCache
participant ServerA as Server A
participant ServerB as Server B
Sender->>HttpSender: SendAsync(payload)
HttpSender->>AddrProv: Get current address
AddrProv-->>HttpSender: hostA:portA
HttpSender->>Cache: GetClientFor(hostA:portA)
Cache-->>HttpSender: HttpClient(hostA)
HttpSender->>ServerA: POST /write
ServerA-->>HttpSender: 500 (retriable)
Note over HttpSender,AddrProv: On retriable error -> rotate and retry
HttpSender->>AddrProv: RotateToNextAddress()
AddrProv-->>HttpSender: hostB:portB
HttpSender->>Cache: GetClientFor(hostB:portB)
Cache-->>HttpSender: HttpClient(hostB)
HttpSender->>ServerB: POST /write
ServerB-->>HttpSender: 200 OK
HttpSender-->>Sender: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
src/dummy-http-server/IlpEndpoint.cs (1)
127-128: Thread safety concern with returned mutable collections.
GetReceiveBufferandGetReceiveBytesreturn mutableStringBuilderandList<byte>without synchronization. Callers modifying these collections concurrently withHandleAsynccould cause race conditions. This is likely acceptable for test infrastructure where tests typically wait for server operations to complete before accessing buffers.Verify that tests consistently await server operations before accessing these buffers to avoid data races.
src/net-questdb-client/Utils/AddressProvider.cs (1)
97-130: IPv6 address parsing may fail.Using
LastIndexOf(':')will incorrectly parse IPv6 addresses like[::1]:9000or::1:9000, treating the final colon as the port separator.Consider handling bracketed IPv6 notation:
public static string ParseHost(string address) { if (string.IsNullOrEmpty(address)) return address; + // Handle bracketed IPv6: [::1]:9000 + if (address.StartsWith("[")) + { + var closeBracket = address.IndexOf(']'); + if (closeBracket > 0) + return address.Substring(1, closeBracket - 1); + } + var index = address.LastIndexOf(':'); if (index > 0) { return address.Substring(0, index); } return address; }src/net-questdb-client/Senders/HttpSender.cs (2)
269-282: Double assignment of_clientfield.
GetClientForCurrentAddressassigns_clientat line 280 and also returns the client. The caller (e.g., line 167, 201) also assigns to_client. This redundancy is harmless but adds confusion.Consider either removing the assignment inside this method or having callers use the return value without reassigning:
private HttpClient GetClientForCurrentAddress() { var address = _addressProvider.CurrentAddress; if (!_clientCache.TryGetValue(address, out var client)) { client = CreateClientForAddress(address); _clientCache[address] = client; } - _client = client; return client; }
288-308: CleanupUnusedClients disposes clients that may have in-flight connections.When a successful response is received,
CleanupUnusedClientsdisposes all HttpClient instances except the current one. However, these clients share the same underlyingSocketsHttpHandler, and disposing them while connections may still be pooled could cause issues. Additionally, if the current address fails on the next request, new clients will need to be recreated for all other addresses.Consider keeping clients alive for a configurable period or only cleaning up on disposal:
- private void CleanupUnusedClients() + // Optional: remove aggressive cleanup to allow faster failover on subsequent failures + private void CleanupUnusedClients() { - if (!_addressProvider.HasMultipleAddresses) - { - return; - } - - var currentAddress = _addressProvider.CurrentAddress; - var addressesToRemove = _clientCache.Keys - .Where(address => address != currentAddress) - .ToList(); - - foreach (var address in addressesToRemove) - { - if (_clientCache.TryGetValue(address, out var client)) - { - client.Dispose(); - _clientCache.Remove(address); - } - } + // Keep all clients alive for potential future failover + // Clients will be disposed in Dispose() }src/net-questdb-client-tests/QuestDbManager.cs (2)
33-39: HttpClient is not disposed if constructor throws.If the constructor throws after creating
_httpClient, the client won't be disposed. While unlikely, consider wrapping in try-catch or using a factory pattern.
228-232: Null-conditional on _httpClient is unnecessary.The
_httpClientfield is assigned in the constructor and is never null. The?.operator on line 231 is defensive but misleading since it suggests the field could be null.public async ValueTask DisposeAsync() { await StopAsync(); - _httpClient?.Dispose(); + _httpClient.Dispose(); }src/net-questdb-client-tests/MultiUrlHttpTests.cs (1)
131-158: Test name suggests round-robin but tests first-address preference.The test
MultipleAddresses_RoundRobinRotationasserts that data goes to server 1, which validates first-address preference rather than round-robin distribution. Consider renaming or adding assertions that verify rotation actually occurs after failures.[Test] - public async Task MultipleAddresses_RoundRobinRotation() + public async Task MultipleAddresses_FirstAddressPreferred() { - // Test round-robin rotation across multiple addresses + // Test that first available address is used when all are healthysrc/net-questdb-client/Utils/SenderOptions.cs (1)
602-606: Consider limiting split to handle edge cases.If an address value ever contains an
=character (though unlikely), the currentSplit('=')would yield more than 2 parts and the address would be silently ignored.Apply this diff for defensive parsing:
- var kvp = param.Split('='); - if (kvp.Length == 2 && kvp[0].Trim() == "addr") + var kvp = param.Split('=', 2); + if (kvp.Length == 2 && kvp[0].Trim().Equals("addr", StringComparison.OrdinalIgnoreCase))Note: Also consider case-insensitive key matching for consistency with typical connection string handling.
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (1)
220-221: Consider usingInterlocked.Incrementinstead of lock for counter.For simple counter increments,
Interlocked.Incrementis more concise and slightly more performant than a lock.Apply this diff:
- var sendLock = new object(); ... - lock (sendLock) - { - batchesSent++; - } + Interlocked.Increment(ref batchesSent);Also applies to: 271-274
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/dummy-http-server/DummyHttpServer.cs(8 hunks)src/dummy-http-server/IlpEndpoint.cs(3 hunks)src/net-questdb-client-tests/HttpTests.cs(10 hunks)src/net-questdb-client-tests/MultiUrlHttpTests.cs(1 hunks)src/net-questdb-client-tests/QuestDbIntegrationTests.cs(1 hunks)src/net-questdb-client-tests/QuestDbManager.cs(1 hunks)src/net-questdb-client/Buffers/BufferV1.cs(1 hunks)src/net-questdb-client/Buffers/IBuffer.cs(1 hunks)src/net-questdb-client/Senders/AbstractSender.cs(1 hunks)src/net-questdb-client/Senders/HttpSender.cs(24 hunks)src/net-questdb-client/Senders/ISender.cs(1 hunks)src/net-questdb-client/Utils/AddressProvider.cs(1 hunks)src/net-questdb-client/Utils/SenderOptions.cs(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-17T16:38:52.474Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In the net-questdb-client project (C#), the NullableColumn pattern (e.g., NullableColumn(ReadOnlySpan<char> name, long? value)) is considered a bad practice and is only kept for backward compatibility reasons. Do not suggest adding NullableColumn overloads for new types.
Applied to files:
src/net-questdb-client/Buffers/IBuffer.cssrc/net-questdb-client/Senders/AbstractSender.cssrc/net-questdb-client/Senders/ISender.cssrc/net-questdb-client/Buffers/BufferV1.cs
📚 Learning: 2025-10-17T16:38:52.474Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In src/net-questdb-client/Senders/ISender.cs, the decimal type intentionally does not have a NullableColumn overload. This is a deliberate design decision because NullableColumn is deprecated in favor of using the Column method directly with nullable types.
Applied to files:
src/net-questdb-client/Buffers/IBuffer.cssrc/net-questdb-client/Senders/AbstractSender.cssrc/net-questdb-client/Senders/ISender.cssrc/net-questdb-client-tests/HttpTests.cssrc/net-questdb-client/Buffers/BufferV1.cs
📚 Learning: 2025-10-17T16:37:03.583Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/dummy-http-server/DummyHttpServer.cs:228-299
Timestamp: 2025-10-17T16:37:03.583Z
Learning: In src/dummy-http-server/DummyHttpServer.cs, the PrintBuffer() method is a test helper that produces human-readable output from received bytes. Decimal tests use binary representation directly via DecimalTestHelpers and do not call PrintBuffer, so DECIMAL type decoding in PrintBuffer is not required.
Applied to files:
src/dummy-http-server/IlpEndpoint.cs
🧬 Code graph analysis (7)
src/net-questdb-client/Senders/HttpSender.cs (1)
src/net-questdb-client/Utils/AddressProvider.cs (5)
AddressProvider(30-131)AddressProvider(39-48)ParsePort(115-130)ParseHost(97-109)RotateToNextAddress(89-92)
src/net-questdb-client/Senders/AbstractSender.cs (1)
src/net-questdb-client/Senders/ISender.cs (16)
ISender(68-68)ISender(109-109)ISender(117-117)ISender(130-130)ISender(138-138)ISender(141-141)ISender(149-149)ISender(157-157)ISender(165-165)ISender(173-173)ISender(181-181)ISender(255-255)ISender(266-266)ISender(274-274)ISender(284-293)ISender(304-312)
src/dummy-http-server/IlpEndpoint.cs (1)
src/dummy-http-server/DummyHttpServer.cs (5)
StringBuilder(199-202)List(208-211)Exception(213-216)GetCounter(244-247)Clear(147-150)
src/net-questdb-client/Senders/ISender.cs (3)
src/net-questdb-client/Senders/AbstractSender.cs (16)
ISender(46-49)ISender(70-74)ISender(77-81)ISender(84-88)ISender(91-95)ISender(103-107)ISender(110-114)ISender(117-121)ISender(124-128)ISender(131-135)ISender(138-142)ISender(144-148)ISender(150-154)ISender(156-160)ISender(276-280)ISender(283-287)src/net-questdb-client/Buffers/IBuffer.cs (1)
ReadOnlySpan(205-205)src/net-questdb-client/Buffers/BufferV1.cs (1)
ReadOnlySpan(195-198)
src/dummy-http-server/DummyHttpServer.cs (1)
src/dummy-http-server/IlpEndpoint.cs (6)
ToString(51-55)IlpEndpoint(82-283)ClearPort(164-175)SetPortConfig(177-183)Exception(130-136)GetCounter(147-153)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (2)
src/net-questdb-client/Senders/HttpSender.cs (4)
Task(415-432)Task(645-659)Task(662-785)TimeSpan(369-373)src/net-questdb-client/Sender.cs (1)
Sender(48-96)
src/net-questdb-client/Buffers/BufferV1.cs (2)
src/net-questdb-client/Buffers/IBuffer.cs (17)
IBuffer(66-66)IBuffer(74-74)IBuffer(84-84)IBuffer(97-97)IBuffer(105-105)IBuffer(113-113)IBuffer(121-121)IBuffer(129-129)IBuffer(137-137)IBuffer(145-145)IBuffer(153-153)IBuffer(228-228)IBuffer(235-235)IBuffer(248-248)IBuffer(261-261)IBuffer(273-273)ReadOnlySpan(205-205)src/net-questdb-client/Utils/IngressError.cs (3)
IngressError(35-66)IngressError(42-46)IngressError(54-58)
🔇 Additional comments (39)
src/dummy-http-server/DummyHttpServer.cs (5)
44-48: LGTM - Instance fields for per-server configuration.These fields properly enable concurrent server instances with independent configuration, supporting the multi-URL testing requirements.
71-84: LGTM - Dual storage approach for backwards compatibility.Configuration is stored in both instance fields (for per-server isolation) and static fields (for backwards compatibility with existing tests). This is a reasonable transition strategy.
117-122: LGTM - Middleware for port identification.The middleware correctly sets the
X-Server-Portheader so endpoints can identify which server instance is handling the request. This is essential for the per-port state isolation implemented inIlpEndpoint.
168-179: LGTM - Per-port configuration and dynamic URL scheme.The port configuration is properly registered with
IlpEndpoint, and the URL scheme correctly uses HTTPS when client certificates are required.
149-150: LGTM - Port-scoped accessor usage.All state accessors (
Clear,GetReceiveBuffer,GetReceivedBytes,GetLastError,GetCounter) correctly use the instance's_portfield to access port-specific data.Also applies to: 201-201, 210-210, 215-215, 246-246
src/dummy-http-server/IlpEndpoint.cs (4)
86-123: LGTM - Port-keyed storage implementation.The port-keyed dictionaries with proper locking enable multiple concurrent
DummyHttpServerinstances. TheGetPortKeyfallback toLocalPortwhen the header is missing provides good resilience.
130-162: LGTM - Port-specific accessors with proper synchronization.
GetLastError,SetLastError,GetCounter, andSetCounterall properly acquire locks before accessing shared state. The pattern of re-fetching data viaGetOrCreatePortDatainside the lock ensures consistency.
164-196: LGTM - Port clearing and configuration management.
ClearPortcorrectly resets buffer state while preserving the container objects.SetPortConfigandGetPortConfigprovide proper isolation with fallback to global defaults for backwards compatibility.
215-259: LGTM - Port-aware request handling.
HandleAsynccorrectly retrieves port from context, uses port-scoped configuration for behavior control (RetriableError,ErrorMessage), and stores request data in port-specific buffers with proper locking.src/net-questdb-client/Utils/AddressProvider.cs (3)
30-34: Thread safety consideration for concurrent access.
_currentIndexis accessed without synchronization. IfRotateToNextAddressorCurrentIndexsetter is called from multiple threads (e.g., during concurrent retry logic), this could lead to race conditions.Verify whether
AddressProvideris accessed from multiple threads. If so, consider usingInterlocked.Incrementfor atomic rotation:public void RotateToNextAddress() { - _currentIndex = (_currentIndex + 1) % _addresses.Count; + Interlocked.Increment(ref _currentIndex); + // Use modulo when reading: _currentIndex % _addresses.Count }
39-48: LGTM - Constructor with validation and defensive copy.Properly validates that at least one address is provided and creates a defensive copy of the input list.
85-92: LGTM - Round-robin rotation logic.The modulo operation correctly wraps around to the beginning of the address list.
src/net-questdb-client/Buffers/IBuffer.cs (1)
299-314: LGTM - New Column overloads for char and Guid.These interface additions properly extend the IBuffer contract to support additional column types. The documentation is clear and consistent with existing patterns. Based on learnings, using direct
Columnoverloads (rather thanNullableColumn) is the correct approach.src/net-questdb-client/Buffers/BufferV1.cs (2)
508-513: LGTM - Efficient char column implementation.Using
stackallocfor a single-character span avoids heap allocation, and delegating to the existingColumn(ReadOnlySpan<char>)reuses the string column encoding logic.
515-525: LGTM - Guid column implementation with defensive validation.The implementation correctly uses
stackallocfor the 36-character buffer required by the default GUID format ("D" format:xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx). The length validation is good defensive programming, thoughTryFormatshould consistently write exactly 36 characters for this format.src/net-questdb-client/Senders/AbstractSender.cs (1)
282-294: LGTM - New Column overloads for Guid and char.Both implementations correctly follow the established pattern of delegating to
Buffer.Columnand returningthisfor fluent API chaining. The use of<inheritdoc />appropriately references the interface documentation.src/net-questdb-client/Senders/HttpSender.cs (3)
570-586: Address rotation occurs before retry delay, which is correct.The rotation logic correctly moves to the next address before sleeping and retrying. This ensures the retry attempt uses a different server.
718-735: Async retry path mirrors sync path correctly.The async
SendAsyncmethod properly implements address rotation on retries, matching the synchronousSendWithRetriesbehavior.
818-831: Dispose properly cleans up all cached clients.The disposal logic correctly iterates through all cached clients and disposes them before clearing the cache and disposing the handler.
src/net-questdb-client-tests/QuestDbManager.cs (1)
197-223: Readiness check is robust with appropriate timeout.The
WaitForQuestDbAsyncmethod polls/settingswith a 30-second timeout, which is reasonable for container startup. The implementation correctly handles exceptions during polling.src/net-questdb-client-tests/MultiUrlHttpTests.cs (2)
76-102: Test properly validates primary address routing.The test correctly verifies that when the primary address is available, data is sent only to that server and not to the secondary.
104-129: Failover test validates retry behavior on retriable errors.The test correctly sets up a server returning retriable errors and verifies failover to the secondary address. The 5-second retry timeout is appropriate.
src/net-questdb-client/Senders/ISender.cs (1)
444-492: New Guid and char column support is consistent with existing patterns.The new
ColumnandNullableColumnoverloads forGuidandcharfollow the established interface patterns. TheNullableColumnimplementations correctly usevalue.Valueinstead of the null-coalescing pattern used in other overloads, which is cleaner.Based on learnings, while
NullableColumnis considered legacy, it's kept for backward compatibility. These additions maintain consistency with the existing API.src/net-questdb-client-tests/HttpTests.cs (5)
1104-1122: GUID column test validates correct wire format.The test correctly verifies that GUIDs are serialized as quoted strings in the ILP wire format.
1124-1141: Char column test validates correct wire format.The test correctly verifies that char values are serialized as quoted single-character strings.
1168-1195: Nullable GUID test covers both value and null cases.The test properly validates that nullable GUIDs are serialized when present and omitted when null.
1197-1222: Nullable char test covers both value and null cases.The test properly validates that nullable chars are serialized when present and omitted when null.
1860-1870: Test refactored to use pre-configured sender correctly.The test now properly builds the sender before making assertions, which is the correct approach.
src/net-questdb-client/Utils/SenderOptions.cs (4)
60-60: LGTM!The field is correctly initialized to an empty list for collecting multiple addresses.
168-182: LGTM!The read-only exposure of the addresses list and the count property are appropriately designed. The
[JsonIgnore]attributes correctly prevent serialization of these internal properties.
707-714: LGTM!The fallback logic correctly ensures that the addresses list is never empty by using the primary address when no addresses were parsed from the config string.
162-166: Potential inconsistency when settingaddrdirectly.When
addris set via the property setter,_addressesis not updated. This could lead toaddressesreturning stale values if users mix property-based and config-string-based configuration.Verify if this is intentional behavior. If
addrshould be the canonical "first" address, consider either:
- Documenting that
addressesis only populated from config strings, or- Synchronizing
_addresses[0]whenaddris set:public string addr { get => _addr; - set => _addr = value; + set + { + _addr = value; + if (_addresses.Count > 0) + _addresses[0] = value; + } }src/net-questdb-client-tests/QuestDbIntegrationTests.cs (7)
37-56: LGTM!The test fixture lifecycle is properly managed with async setup and teardown. The null check in
TearDownFixturecorrectly handles cases where setup might have failed.
58-153: LGTM!The basic integration tests cover HTTP, TCP/ILP, multi-row, and multi-type scenarios effectively. The retry logic in
CanSendDifferentDataTypesappropriately handles eventual consistency.
155-176: LGTM!Good test for validating the multi-URL fallback feature. The test correctly configures a non-existent endpoint first and expects the client to fall back to the valid endpoint.
Minor note: Port 19001 is assumed to be unused. If this proves flaky in CI environments, consider using a guaranteed-invalid address like
localhost:1(reserved port).
349-356: Same volume cleanup concern applies here.Both
volume1andvolume2are created but cleanup isn't visible. See previous comment about verifyingQuestDbManager.DisposeAsync()handles volume removal.
516-520: Verify expected row count behavior during failover.The assertion expects exactly
expectedTotalRowsacross both databases. This assumes:
- No duplicate rows (each batch sent exactly once)
- No data loss during failover
Is this the expected behavior? If retries could cause duplicate writes to different databases, consider using
Is.GreaterThanOrEqualTolike the single-database chaos test.
538-580: LGTM!The helper methods provide good retry logic for eventual consistency. The
Assert.Failat line 578 will throw, making thereturn 0unreachable, but this pattern satisfies the compiler's return requirement.
1-35: Good integration test coverage for multi-URL support.The test suite provides comprehensive coverage of:
- Basic HTTP and ILP data ingestion
- Multi-URL failover scenarios
- Auto-flush behavior
- Chaos testing with database restarts
- Multi-database failover
The tests are well-structured with proper setup/teardown and will help catch regressions in the new multi-URL feature.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/net-questdb-client-tests/QuestDbManager.cs (2)
110-156: Handle startup failures by cleaning up the container and normalizingIsRunning.If
WaitForQuestDbAsyncthrows (timeout or other failure), the container will stay running until a later cleanup, andIsRunningremainstrueeven though startup failed. This can leave stray containers and make test behavior surprising.Consider wrapping the readiness wait in a
try/catchthat attempts best-effort cleanup and restoresIsRunningtofalse:_containerId = output.Trim(); Console.WriteLine($"QuestDB container started: {_containerId}"); - IsRunning = true; - - // Wait for QuestDB to be ready - await WaitForQuestDbAsync(); + IsRunning = true; + + try + { + // Wait for QuestDB to be ready + await WaitForQuestDbAsync(); + } + catch + { + // Best-effort cleanup if startup fails + try + { + await StopAsync(); + } + catch + { + // Ignore cleanup failures + } + + throw; + }
265-291: Dispose theProcessto avoid leaking OS handles.
RunDockerCommandAsyncstarts aProcessbut never disposes it, which can leak handles over many test runs. The concurrent reading of stdout/stderr looks good (and fixes the earlier deadlock risk), but theProcessshould be wrapped in ausingblock.Example refactor:
- var process = Process.Start(startInfo); - if (process == null) - { - throw new InvalidOperationException("Failed to start docker command"); - } - - var outputTask = process.StandardOutput.ReadToEndAsync(); - var errorTask = process.StandardError.ReadToEndAsync(); - await Task.WhenAll(outputTask, errorTask); - var output = await outputTask; - var error = await errorTask; - await process.WaitForExitAsync(); - - return (process.ExitCode, output + error); + var process = Process.Start(startInfo); + if (process == null) + { + throw new InvalidOperationException("Failed to start docker command"); + } + + using (process) + { + var outputTask = process.StandardOutput.ReadToEndAsync(); + var errorTask = process.StandardError.ReadToEndAsync(); + + await Task.WhenAll(outputTask, errorTask); + await process.WaitForExitAsync(); + + var output = await outputTask; + var error = await errorTask; + + return (process.ExitCode, output + error); + }src/net-questdb-client/Senders/HttpSender.cs (2)
253-274: Clarify ownership ofSocketsHttpHandlerto avoid double-dispose and simplify cleanupRight now:
CreateClientForAddressdoesnew HttpClient(handler)(defaultdisposeHandler: true).CleanupUnusedClientsandDisposeboth explicitly callhandler.Dispose()from_handlerCacheas well.That means each handler is typically disposed twice (once via
HttpClient.Dispose(), once directly). This is harmless in practice, but it complicates reasoning about ownership.Consider simplifying one of these ways:
- Let
HttpClientown the handler
- Keep
new HttpClient(handler)as-is.- In
CleanupUnusedClientsandDispose, only dispose clients and just remove handlers from_handlerCachewithout disposing them explicitly:
if (_handlerCache.TryGetValue(address, out var handler)){handler.Dispose();_handlerCache.Remove(address);}
if (_handlerCache.ContainsKey(address)){_handlerCache.Remove(address);}And in `Dispose()`: ```diff
foreach (var handler in _handlerCache.Values){handler.Dispose();}_handlerCache.Clear();
_handlerCache.Clear();
- Or make
HttpSenderown the handlers explicitly
- Construct clients with
new HttpClient(handler, disposeHandler: false)and keep the explicit handler disposal you already have.Either option makes the lifetime model clearer and avoids redundant disposal logic.
Also applies to: 294-324, 833-851
529-626: Deduplicate sync/async retry & rotation logic to reduce divergence risk
SendWithRetries(sync) andSendAsyncboth implement:
- Exponential backoff with jitter,
- Rotation via
_addressProvider.RotateToNextAddress(),- Per-address client retrieval via
GetClientForCurrentAddress(),- Handling of
HttpRequestException+IsRetriableErrorstatus codes.The logic is nearly identical but duplicated, which makes future changes to retry semantics or address-rotation behavior easy to get out of sync between sync and async paths.
Consider factoring the common pieces into a shared helper (e.g., a small state machine that takes
Func<HttpRequestMessage>and a delegate to perform the actual send) and havingSend/SendAsynccall the same core implementation. That would keep the multi-URL failover behavior consistent and easier to maintain.Also applies to: 677-801
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/net-questdb-client-tests/QuestDbManager.cs(1 hunks)src/net-questdb-client/Senders/HttpSender.cs(23 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/net-questdb-client-tests/QuestDbManager.cs (2)
src/net-questdb-client/Senders/HttpSender.cs (7)
HttpClient(227-274)HttpClient(279-292)TimeSpan(385-389)Task(431-448)Task(661-675)Task(678-801)Dispose(834-853)src/net-questdb-client-tests/QuestDbIntegrationTests.cs (2)
Task(532-536)Task(538-580)
src/net-questdb-client/Senders/HttpSender.cs (2)
src/net-questdb-client/Utils/AddressProvider.cs (5)
AddressProvider(30-131)AddressProvider(39-48)ParsePort(115-130)ParseHost(97-109)RotateToNextAddress(89-92)src/net-questdb-client/Utils/SenderOptions.cs (4)
SenderOptions(92-95)SenderOptions(101-128)SenderOptions(732-738)ToString(649-694)
🔇 Additional comments (1)
src/net-questdb-client/Senders/HttpSender.cs (1)
47-58: Per-address HttpClient/SocketsHttpHandler caching and rotation look solidThe new
_clientCache/_handlerCacheplusAddressProvider-driven rotation andCleanupUnusedClientsgive you:
- One
SocketsHttpHandlerper address with a fixedSslOptions.TargetHost(no more cross-address mutation).- A cached
HttpClientper address, re-used across retries and sends.- Cleanup of non-current clients/handlers after a successful send to avoid resource bloat.
This addresses the earlier concern about mutating a shared handler’s
TargetHostacross different endpoints and aligns well with the documented single-threaded sender usage.Also applies to: 114-169, 222-274, 294-325
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/net-questdb-client-tests/QuestDbManager.cs (1)
255-272: Trim container names to avoid cleanup failures on Windows.The code splits container names on
'\n'but doesn't trim the resulting strings. On Windows,docker psoutput uses\r\nline endings, which will leave a trailing\rin each name. This can causedocker stopanddocker rmcommands to fail because the container name won't match exactly, potentially leaving containers running and causing port conflicts in subsequent test runs.Apply this fix to trim names and use ordinal string comparisons:
var containerNames = output.Split('\n', StringSplitOptions.RemoveEmptyEntries); // Stop and remove any QuestDB test containers -foreach (var name in containerNames) +foreach (var rawName in containerNames) { + var name = rawName.Trim(); + // Look for containers with matching port pattern: questdb-test-{port}-{httpPort}-* - if (name.Contains("questdb-test-") && - (name.Contains($"-{_port}-{_httpPort}-") || name.Contains($"-{_httpPort}-{_port}-"))) + if (name.StartsWith(ContainerNamePrefix, StringComparison.Ordinal) && + (name.Contains($"-{_port}-{_httpPort}-", StringComparison.Ordinal) || + name.Contains($"-{_httpPort}-{_port}-", StringComparison.Ordinal))) { Console.WriteLine($"Cleaning up existing container: {name}"); // Stop the container await RunDockerCommandAsync($"stop -t 5 {name}"); // Remove the container await RunDockerCommandAsync($"rm {name}"); } }Based on past review feedback.
🧹 Nitpick comments (1)
src/net-questdb-client-tests/MultiUrlHttpTests.cs (1)
131-158: Test name and comment don't match the implementation.The test is named
MultipleAddresses_RoundRobinRotationand the comment states "Test round-robin rotation across multiple addresses," but the test only sends a single request and verifies it goes to server 1. This doesn't actually test rotation behavior. To properly test round-robin rotation, the test would need to either send multiple requests and verify distribution, or demonstrate failover to subsequent servers.Consider either renaming this test to something like
MultipleAddresses_UsesFirstServerWhenAllAvailableor expanding it to actually test rotation across multiple send operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/net-questdb-client-tests/MultiUrlHttpTests.cs(1 hunks)src/net-questdb-client-tests/QuestDbManager.cs(1 hunks)src/net-questdb-client/Utils/AddressProvider.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/net-questdb-client-tests/MultiUrlHttpTests.cs (2)
src/net-questdb-client/Sender.cs (1)
Sender(48-96)src/net-questdb-client/Utils/AddressProvider.cs (3)
AddressProvider(30-166)AddressProvider(39-48)RotateToNextAddress(89-92)
src/net-questdb-client-tests/QuestDbManager.cs (1)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (2)
Task(532-536)Task(538-580)
🔇 Additional comments (3)
src/net-questdb-client-tests/MultiUrlHttpTests.cs (1)
290-337: IPv6 parsing tests look comprehensive.The IPv6 address parsing tests properly exercise the
AddressProvider.ParseHostandParsePortmethods with various formats including[::1]:9000,[2001:db8::1]:9000, and addresses without ports. The implementation inAddressProvider.cscorrectly handles IPv6 by detecting the opening bracket and finding the closing bracket before parsing the port, which addresses the concern raised in the past review about incorrect parsing usingLastIndexOf(':').src/net-questdb-client-tests/QuestDbManager.cs (1)
293-298: Process stream handling correctly prevents deadlocks.The concurrent reading of
StandardOutputandStandardErrorusingTask.WhenAllproperly avoids potential deadlocks that could occur if one stream's buffer fills before the other is read. This implementation correctly addresses the concern from the previous review.src/net-questdb-client/Utils/AddressProvider.cs (1)
99-165: IPv6 address parsing correctly implemented.The
ParseHostandParsePortmethods properly handle IPv6 addresses in bracket notation by:
- Detecting the opening
[bracket- Finding the closing
]bracket first- Returning the entire bracketed section as the host (including both brackets)
- Parsing the port only from the text after the closing bracket
This approach correctly avoids the issue mentioned in past reviews where using
LastIndexOf(':')directly would split inside the IPv6 address itself. For example,[::1]:9000now correctly parses to host[::1]and port9000, rather than the broken result mentioned in the earlier review.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/net-questdb-client/Senders/HttpSender.cs (1)
223-245: Simplify and harden host parsing inCreateClientForAddressThe
Contains("//")+Split("//")[1]path is brittle and can throw for some inputs (e.g., a full URI without an explicit port such as"https://example.com"causesParseHostto return"https", andSplit("//")[1]thenIndexOutOfRangeException). SinceAddressProvider.ParseHostalready handles the supportedhost:portand[ipv6]:portformats, this special-case branch isn’t really buying anything and makes misconfiguration fail with an opaque runtime exception.You can avoid the edge cases and simplify the code by always using
ParseHostdirectly:- var host = address.Contains("//") - ? AddressProvider.ParseHost(address).Split("//")[1] - : AddressProvider.ParseHost(address); + var host = AddressProvider.ParseHost(address);This keeps behavior predictable for the documented address formats and degrades misconfigurations into cleaner errors rather than crashes.
🧹 Nitpick comments (3)
src/net-questdb-client/Senders/HttpSender.cs (3)
276-324: Cache cleanup strategy is correct but may be a bit aggressive for frequent failover
GetClientForCurrentAddressandCleanupUnusedClientscorrectly ensure that each address has at most oneHttpClient/SocketsHttpHandlerpair and that non-current entries are disposed once a request finally succeeds. That said, in environments where writers routinely fail over between multiple URLs, eagerly dropping all non-current clients/handlers on every successful write could lead to extra churn (re-creating handlers, redoing TLS handshakes, etc.) on subsequent failbacks.If you expect frequent oscillation between endpoints, consider softening the cleanup policy (e.g., only trimming on
Dispose, or trimming least-recently-used entries above some cap) to trade a small amount of memory for fewer handler/client re-creations. Current behavior is still correct, so this is just a perf/operational tuning suggestion.
462-527: Sync/async send paths rotate addresses correctly; minor unification opportunityThe updated
Send/SendAsyncandSendWithRetriespaths now:
- Respect the transaction/empty-buffer guards as before.
- Use a per-request CTS with
CalculateRequestTimeout().- On retriable failures, back off with jitter and, when multiple addresses are configured, rotate via
AddressProviderand callGetClientForCurrentAddress()before the next attempt.- Surface a clearer “Cannot connect to
host:port” message usingCurrentHost/CurrentPort.- Call
CleanupUnusedClients()on success to shed non-current address resources.This all looks logically consistent for both sync and async flows.
One small unification you might consider (purely for clarity/future-proofing) is to also use
GetClientForCurrentAddress()for the very first attempt instead of_clientdirectly, so every attempt—first and subsequent—goes through the same mechanism to resolve the active client from the current address. Today_clientand_addressProviderare kept in sync so this isn’t a correctness issue, but unifying the pattern would reduce the chance of future refactors accidentally desynchronizing them.Also applies to: 529-626, 677-801
803-831: Re-trying 404/421 might be too aggressive for single-address setupsMarking
404and421as retriable makes sense in multi-URL deployments where one instance might not accept writes or might be misdirected, so rotating to another address is desirable. However, in single-address configurations this will cause the client to resend the same request to the same endpoint untilretry_timeoutelapses, even though a 404/421 from that one server is rarely recoverable and is more likely a hard configuration or routing problem.To keep the benefits for multi-URL failover while avoiding unnecessary retries in the single-URL case, you could gate the 404/421 behavior on the presence of multiple addresses. For example:
- Keep
IsRetriableErrorfocused on clearly transient/server-side issues (5xx, 509, 523, 524, 529, 599).- Treat 404/421 as “conditionally retriable” only when
_addressProvider.HasMultipleAddressesis true, by checking them at the call site before deciding to enter the retry loop.This would improve fail-fast behavior for simple single-endpoint deployments without changing semantics for the multi-URL use case you’re targeting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/net-questdb-client-tests/QuestDbManager.cs(1 hunks)src/net-questdb-client/Senders/HttpSender.cs(23 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/net-questdb-client-tests/QuestDbManager.cs
🔇 Additional comments (3)
src/net-questdb-client/Senders/HttpSender.cs (3)
47-58: Per-address client/handler caching and Build() initialization look solidThe per-address
_clientCache/_handlerCacheplusCreateHandler/Buildwiring correctly avoid the previous shared-handler TLSTargetHostissue and cleanly encapsulate per-host TLS settings and pool configuration. AddressProvider integration inBuild()(including saving/restoringCurrentIndexaround the/settingsprobe and refreshing_client) keeps the active address and client in sync without leaking probe-induced rotations.Also applies to: 114-220
629-675: JSON error handling is clear and robustBoth sync and async JSON error handlers now consistently deserialize into
JsonErrorResponsewhen possible and fall back to streaming the raw body when deserialization fails, composing a singleIngressErrormessage withReasonPhraseplus details. The async version’s expliciterrorStrlocal avoids double-reading the stream and keeps disposal semantics correct.
833-853: Disposal of per-address clients and handlers is correctThe new
Disposeimplementation that walks_clientCacheand_handlerCache, disposing and clearing both, ensures all per-address resources are released, on top of the existing buffer cleanup. Given that this sender isn’t intended to be reused after disposal, this is an appropriate and complete teardown.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dummy-http-server/dummy-http-server.csproj (1)
17-47: Remove build artifacts from the project file.Lines 17-47 include generated build artifacts (obj/Debug paths, cache files, compiled assemblies) that should not be tracked in source control. These files are produced during compilation and will vary across machines and builds, leading to unnecessary merge conflicts and repository bloat.
Apply this diff to remove the artifact references:
</ItemGroup> - <ItemGroup> - <Content Include="obj\Debug\net6.0\apphost.exe"/> - <Content Include="obj\Debug\net6.0\dummy-http-server.AssemblyInfoInputs.cache"/> - <Content Include="obj\Debug\net6.0\dummy-http-server.assets.cache"/> - <Content Include="obj\Debug\net6.0\dummy-http-server.csproj.CoreCompileInputs.cache"/> - <Content Include="obj\Debug\net6.0\dummy-http-server.csproj.FileListAbsolute.txt"/> - <Content Include="obj\Debug\net6.0\dummy-http-server.dll"/> - <Content Include="obj\Debug\net6.0\dummy-http-server.genruntimeconfig.cache"/> - <Content Include="obj\Debug\net6.0\dummy-http-server.MvcApplicationPartsAssemblyInfo.cache"/> - <Content Include="obj\Debug\net6.0\dummy-http-server.pdb"/> - <Content Include="obj\Debug\net6.0\dummy-http-server.sourcelink.json"/> - <Content Include="obj\Debug\net6.0\staticwebassets.build.json"/> - <Content Include="obj\dummy-http-server.csproj.nuget.dgspec.json"/> - <Content Include="obj\project.assets.json"/> - <Content Include="obj\project.nuget.cache"/> - <Content Include="obj\project.packagespec.json"/> - <Content Include="obj\rider.project.restore.info"/> - <Content Remove="obj\**"/> - </ItemGroup> - - <ItemGroup> - <Compile Include="obj\Debug\net6.0\.NETCoreApp,Version=v6.0.AssemblyAttributes.cs"/> - <Compile Remove="obj\**"/> - </ItemGroup> - - <ItemGroup> - <EmbeddedResource Remove="obj\**"/> - </ItemGroup> - - <ItemGroup> - <None Remove="obj\**"/> - </ItemGroup> - </Project>
🧹 Nitpick comments (5)
src/net-questdb-client/Senders/HttpSender.cs (1)
171-212: Remove unused variable or clarify intent.Line 185 declares
initialAddressIndexwith a comment suggesting it will be used to restore the address after probing, but the variable is never used. If the intent is to keep whichever address succeeds during the protocol version probe, consider removing the variable and updating the comment. If restoration is intended, implement it.Apply this diff to remove the unused variable and clarify the comment:
if (protocolVersion == ProtocolVersion.Auto) { // We need to select the last version that both client and server support. // Other clients use 1 second timeout for "/settings", follow same practice here. - // Save the current address index to restore after probing (SendWithRetries may rotate) - var initialAddressIndex = _addressProvider.CurrentIndex; + // SendWithRetries may rotate addresses during probing; we keep whichever succeeds try {src/net-questdb-client-tests/QuestDbIntegrationTests.cs (4)
520-527: Remove redundant variable recalculation.
totalRowCount2recalculates the same sum already computed astotalRowCountinside the loop (line 504). Consider reusing the existing variable or moving the final calculation outside the loop.- var totalRowCount2 = count1 + count2; - // Assert that the sum of both databases equals expected total Assert.That( - totalRowCount2, + count1 + count2, Is.EqualTo(expectedTotalRows), - $"Expected {expectedTotalRows} total rows across both databases but found {totalRowCount2}. " + + $"Expected {expectedTotalRows} total rows across both databases but found {count1 + count2}. " + $"Successfully sent {batchesSent} batches of {rowsPerBatch} rows each");
576-579: Empty catch block loses diagnostic information.The empty catch block swallows all exceptions without logging, making it harder to diagnose intermittent failures. Consider adding minimal logging to aid debugging.
catch { - // Retry + // Retry - exception details available via TestContext if needed }Alternatively, capture and log the exception type for debugging intermittent failures:
- catch + catch (Exception ex) { - // Retry + TestContext.WriteLine($"Retry {attempts + 1}: {ex.GetType().Name}"); }
585-586: Dead code after Assert.Fail().
Assert.Fail()throws anAssertionException, soreturn 0on line 586 is unreachable. Consider removing it or changing the method signature to not require a return value here.Assert.Fail($"Table {tableName} has no data after {maxAttempts} attempts"); - return 0; + return 0; // Unreachable, but required for compilationOr throw explicitly to clarify intent:
- Assert.Fail($"Table {tableName} has no data after {maxAttempts} attempts"); - return 0; + throw new AssertionException($"Table {tableName} has no data after {maxAttempts} attempts");
45-52: Simplify teardown by calling onlyDisposeAsync().Since
DisposeAsync()internally callsStopAsync(), the explicit call toStopAsync()on line 50 is redundant. Remove theStopAsync()call and rely onDisposeAsync()alone to handle both stopping and cleanup operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ci/azure-pipelines.yml(1 hunks)global.json(1 hunks)src/dummy-http-server/dummy-http-server.csproj(1 hunks)src/example-aot/example-aot.csproj(1 hunks)src/example-auth-http-tls/example-auth-http-tls.csproj(1 hunks)src/example-auth-tls/example-auth-tls.csproj(1 hunks)src/example-basic/example-basic.csproj(1 hunks)src/example-streaming/example-streaming.csproj(1 hunks)src/net-questdb-client-benchmarks/net-questdb-client-benchmarks.csproj(2 hunks)src/net-questdb-client-tests/HttpTests.cs(10 hunks)src/net-questdb-client-tests/QuestDbIntegrationTests.cs(1 hunks)src/net-questdb-client-tests/QuestDbManager.cs(1 hunks)src/net-questdb-client-tests/SenderOptionsTests.cs(1 hunks)src/net-questdb-client-tests/net-questdb-client-tests.csproj(1 hunks)src/net-questdb-client/Senders/HttpSender.cs(23 hunks)src/net-questdb-client/net-questdb-client.csproj(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/example-streaming/example-streaming.csproj
🚧 Files skipped from review as they are similar to previous changes (1)
- ci/azure-pipelines.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-17T16:38:52.474Z
Learnt from: RaphDal
Repo: questdb/net-questdb-client PR: 52
File: src/net-questdb-client/Senders/ISender.cs:410-418
Timestamp: 2025-10-17T16:38:52.474Z
Learning: In src/net-questdb-client/Senders/ISender.cs, the decimal type intentionally does not have a NullableColumn overload. This is a deliberate design decision because NullableColumn is deprecated in favor of using the Column method directly with nullable types.
Applied to files:
src/net-questdb-client-tests/HttpTests.cs
🧬 Code graph analysis (4)
src/net-questdb-client-tests/QuestDbManager.cs (2)
src/net-questdb-client/Senders/HttpSender.cs (6)
HttpClient(219-264)HttpClient(269-282)TimeSpan(375-379)Task(421-438)Task(651-665)Task(668-791)src/net-questdb-client-tests/QuestDbIntegrationTests.cs (2)
Task(539-543)Task(545-587)
src/net-questdb-client-tests/SenderOptionsTests.cs (1)
src/net-questdb-client/Utils/SenderOptions.cs (3)
SenderOptions(92-95)SenderOptions(101-128)SenderOptions(732-738)
src/net-questdb-client-tests/HttpTests.cs (5)
src/net-questdb-client/Senders/HttpSender.cs (1)
Build(171-212)src/dummy-http-server/DummyHttpServer.cs (3)
DummyHttpServer(36-334)DummyHttpServer(59-132)PrintBuffer(254-333)src/net-questdb-client/Sender.cs (1)
Sender(48-96)src/dummy-http-server/SettingsEndpoint.cs (1)
Configure(10-14)src/net-questdb-client/Senders/TcpSender.cs (1)
Build(74-134)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (2)
src/net-questdb-client/Senders/HttpSender.cs (6)
Task(421-438)Task(651-665)Task(668-791)HttpClient(219-264)HttpClient(269-282)TimeSpan(375-379)src/net-questdb-client/Sender.cs (1)
Sender(48-96)
🔇 Additional comments (25)
src/example-auth-tls/example-auth-tls.csproj (1)
10-10: LGTM!Adding net10.0 target framework is consistent with the broader multi-targeting strategy.
src/example-basic/example-basic.csproj (1)
10-10: LGTM!Consistent with the repository-wide net10.0 adoption.
src/example-auth-http-tls/example-auth-http-tls.csproj (1)
5-5: LGTM!Consistent with the repository-wide net10.0 adoption.
src/net-questdb-client-benchmarks/net-questdb-client-benchmarks.csproj (1)
5-5: LGTM!Framework update to net10.0 and the ProjectReference cleanup are appropriate.
Also applies to: 18-18
src/net-questdb-client/net-questdb-client.csproj (1)
9-9: LGTM! Framework updates look good.Using
LangVersion=latestfor the main library allows leveraging new C# language features, and addingnet10.0to the target frameworks extends support appropriately.global.json (1)
1-7: LGTM! SDK upgrade to .NET 10.0 is consistent with framework targeting.The SDK version upgrade aligns with the repository-wide update to target net10.0 across all projects.
src/example-aot/example-aot.csproj (1)
5-5: LGTM! Framework upgrade is consistent.The update to net10.0 aligns with the global SDK upgrade and broader repository changes.
src/net-questdb-client-tests/net-questdb-client-tests.csproj (1)
8-8: LGTM! Multi-targeting extended to net10.0.The test project now targets net10.0 alongside existing frameworks, consistent with the SDK upgrade.
src/net-questdb-client-tests/SenderOptionsTests.cs (1)
35-46: LGTM! Test validates default port assignment.The test correctly validates that when no port is specified, the default ports (9000 for HTTP, 9009 for TCP) are properly assigned by
SenderOptions.Build(). The test uses proper disposal patterns and clear assertions.src/net-questdb-client/Senders/HttpSender.cs (4)
219-264: LGTM! Per-address client creation with proper handler isolation.The implementation correctly:
- Parses port with protocol-based defaults (9000 for HTTP/HTTPS, 9009 for TCP/TCPS)
- Creates isolated handlers per address to prevent TLS
TargetHostconflicts during rotation- Applies authentication headers consistently
- Caches clients to avoid recreation overhead
This addresses previous review concerns about shared handler conflicts.
534-616: LGTM! Address rotation on retriable errors provides failover.The retry logic correctly rotates to the next address when multiple addresses are configured and a retriable error occurs (connection failure or retriable HTTP status code). The implementation:
- Checks
HasMultipleAddressesbefore rotating- Calls
RotateToNextAddress()to advance the round-robin- Switches to the appropriate cached client via
SwitchClientToCurrentAddress()- Retries the request with exponential backoff and jitter
This implements the multi-URL failover behavior described in the PR objectives.
824-843: LGTM! Proper resource disposal for all cached clients and handlers.The
Disposemethod correctly disposes all cachedHttpClientandSocketsHttpHandlerinstances and clears both caches. Iterating over.Valuesavoids the collection-modification issue.
47-70: Add synchronization to cache dictionaries to prevent race conditions on concurrent SendAsync calls.The
_clientCacheand_handlerCacheare modified without synchronization inSwitchClientToCurrentAddress(adds entries) andCleanupUnusedClients(removes entries). IfSendAsync()is awaited concurrently from multiple threads, concurrent dictionary access could cause race conditions. Consider usingConcurrentDictionary<string, T>or protecting these operations with a lock.src/net-questdb-client-tests/QuestDbManager.cs (3)
26-32: LGTM! HttpClient timeout is appropriate for health check probing.The 5-second timeout per HTTP request combined with the 30-attempt retry loop in
WaitForQuestDbAsyncprovides robust health checking. Each probe times out quickly if QuestDB isn't ready, avoiding long hangs on individual requests.
244-277: LGTM! Container cleanup properly handles cross-platform line endings.The implementation correctly:
- Trims each container name to remove trailing
\ror whitespace (line 261)- Uses
StringComparison.Ordinalfor container name matching- Uses the
ContainerNamePrefixconstant for consistencyThis addresses previous review feedback about Windows line ending issues.
279-305: LGTM! Process output streams read concurrently to prevent deadlock.Lines 297-302 correctly read
StandardOutputandStandardErrorconcurrently usingTask.WhenAll, preventing the deadlock that could occur with sequentialReadToEndAsynccalls. This addresses previous review feedback.src/net-questdb-client-tests/HttpTests.cs (4)
35-35: LGTM! Culture setting addresses issue #50.The
[SetCulture("en-us")]attribute ensures that numeric formatting in tests produces consistent, locale-independent output. This directly addresses Issue #50 where tests failed on systems using,as a decimal separator.
1104-1223: LGTM! Comprehensive test coverage for new Guid and char column types.Lines 1104-1223 add five well-structured tests validating:
- Basic Guid column serialization (formatted as string)
- Basic char column serialization (formatted as string)
- Multiple Guid and char columns in the same row
- Nullable Guid columns (with value and null cases)
- Nullable char columns (with value and null cases)
The tests verify correct ILP formatting, proper null handling (fields omitted when null), and follow consistent patterns with proper server lifecycle management.
427-507: LGTM! Authentication tests simplified and improved cleanup.The authentication tests have been updated to:
- Use plain HTTP transport (removing
tls_verify=unsafe_off)- Add proper server lifecycle cleanup with
await server.StopAsync()These changes simplify the tests while maintaining validation of Basic and Bearer token authentication mechanisms.
1861-1873: LGTM! Test now validates actual send operation without client certificate.The test has been enhanced to:
- Actually prepare and send data (lines 1863-1869) before asserting the error
- Add proper server cleanup with
StopAsync()This makes the test more realistic by validating that the TLS handshake failure occurs during an actual send operation when a client certificate is required but not provided.
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (5)
1-29: LGTM!License header, imports, and namespace declaration are appropriate for an integration test file.
59-93: LGTM!Both HTTP and ILP tests follow a clean pattern with proper resource disposal via
usingdeclarations, explicit flush control, and verification through the helper method.
95-154: LGTM!Both tests are well-structured. The explicit retry logic in
CanSendDifferentDataTypes(lines 141-151) provides clear test flow, though it duplicates the pattern inGetTableRowCountAsync. This is acceptable for test readability.
179-198: LGTM!The auto-flush test correctly configures
auto_flush_rows=1to trigger immediate flush and includes a brief delay before verification. This is a pragmatic approach for testing auto-flush behavior.
200-344: LGTM!This chaos test is well-designed with proper isolation (separate ports), persistent volumes for restart survival, and concurrent task coordination. The thread synchronization with
sendLockis correct, and the retry logic for verification is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/net-questdb-client-tests/MultiUrlHttpTests.cs (1)
299-315: LGTM!Based on the
AddressProvidercode snippets showing proper IPv6 bracket handling inParseHostandParsePort, the IPv6 parsing issue raised in past reviews appears to be resolved. The test correctly validates IPv4, domain, and IPv6 bracketed formats.
🧹 Nitpick comments (3)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (3)
95-116: Consider using exact row count assertion.Line 115 uses
Is.GreaterThanOrEqualTo(10)but since each test uses a unique table name (test_multiple_rows), the count should be exactly 10. UsingIs.EqualTo(10)would catch over-insertion bugs.- Assert.That(rowCount, Is.GreaterThanOrEqualTo(10), "Expected at least 10 rows"); + Assert.That(rowCount, Is.EqualTo(10), "Expected exactly 10 rows");
220-223: Consider usingInterlocked.Incrementinstead of lock for simple counter.The
sendLockobject andlockblock are used only to incrementbatchesSent. This can be simplified using atomic operations.- var batchesSent = 0; - var sendLock = new object(); + var batchesSent = 0; ... - lock (sendLock) - { - batchesSent++; - } + Interlocked.Increment(ref batchesSent);
374-377: Same suggestion: useInterlocked.Incrementfor counter.- var batchesSent = 0; - var sendLock = new object(); + var batchesSent = 0; ... - lock (sendLock) - { - batchesSent++; - } + Interlocked.Increment(ref batchesSent);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/net-questdb-client-tests/MultiUrlHttpTests.cs(1 hunks)src/net-questdb-client-tests/QuestDbIntegrationTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (4)
src/net-questdb-client-tests/QuestDbManager.cs (11)
Task(63-82)Task(87-104)Task(109-118)Task(123-169)Task(174-195)Task(216-242)Task(244-277)Task(279-305)QuestDbManager(9-306)QuestDbManager(26-32)GetHttpEndpoint(200-203)src/net-questdb-client/Senders/HttpSender.cs (6)
Task(421-438)Task(651-665)Task(668-791)HttpClient(219-264)HttpClient(269-282)TimeSpan(375-379)src/net-questdb-client-tests/MultiUrlHttpTests.cs (15)
Test(43-54)Test(56-64)Test(66-74)Test(76-104)Test(106-132)Test(134-162)Test(164-180)Test(182-212)Test(214-240)Test(242-269)Test(271-297)Test(299-315)Test(317-346)Test(348-361)Test(363-432)src/net-questdb-client/Sender.cs (1)
Sender(48-96)
src/net-questdb-client-tests/MultiUrlHttpTests.cs (3)
src/net-questdb-client/Sender.cs (1)
Sender(48-96)src/net-questdb-client/Utils/AddressProvider.cs (3)
AddressProvider(30-166)AddressProvider(39-48)RotateToNextAddress(89-92)src/net-questdb-client/Senders/HttpSender.cs (1)
SocketsHttpHandler(114-169)
🔇 Additional comments (19)
src/net-questdb-client-tests/MultiUrlHttpTests.cs (12)
36-42: LGTM!Class structure and constants are well-defined. The port constants provide clear, consistent values for test server configuration.
43-54: LGTM!Clear test for multi-address config string parsing with appropriate assertions.
56-74: LGTM!Good coverage of single-address and default-address scenarios.
76-104: LGTM!Well-structured test validating primary address routing with proper resource management via
usingstatements.
106-132: LGTM!Effective failover test using retriable error simulation. The test correctly validates that data reaches the secondary server after the primary fails.
164-180: LGTM!Good negative test case verifying proper error handling when all addresses are unavailable.
182-212: LGTM!Effective test demonstrating that address ordering in config determines primary routing.
214-240: LGTM!Important coverage for the synchronous send path with multi-address configuration.
242-269: LGTM!Explicit test for confirming no rotation on successful first attempt. While similar to
MultipleAddresses_SendToFirstAddress, it clearly documents the expected behavior.
271-297: LGTM!Thorough unit test for AddressProvider rotation mechanics, including wrap-around behavior.
317-346: LGTM!Comprehensive IPv6 parsing coverage including edge cases like missing ports and various address formats.
348-361: LGTM!Good boundary test for single-address provider behavior.
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (7)
25-29: LGTM!Imports are minimal and appropriate for the integration tests.
35-57: LGTM!Test fixture structure is appropriate with clearly defined port constants.
38-53: LGTM!Setup and teardown are correctly structured with proper null checking and cleanup sequence.
59-93: LGTM!HTTP and ILP tests are well-structured with proper resource cleanup via
usingstatements and unique table names.
156-177: LGTM!Multi-URL fallback test correctly configures a bad endpoint first, then the valid endpoint, and verifies data is written via failover. The
badEndpointformat is now correct per the previous review.
179-198: LGTM with minor flakiness concern.The auto-flush test correctly configures
auto_flush_rows=1. The 100ms delay before verification may be insufficient on slow CI environments; consider increasing to 500ms if flakiness is observed.
545-587: LGTM with minor observation.The helper methods are well-structured with appropriate retry logic. The table name in the query URL (line 559) is not URL-encoded, but since all test table names are simple alphanumeric strings with underscores, this is acceptable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/net-questdb-client-tests/MultiUrlHttpTests.cs (1)
134-175: Test now properly verifies rotation on failover.The test has been improved to verify actual rotation behavior by stopping servers sequentially. The name
RoundRobinRotationis still slightly misleading since it tests "rotation on failover" rather than "rotation on every request," but this is now a minor naming concern rather than a functional gap.As noted in a previous review, consider renaming to
MultipleAddresses_RotatesOnFailoverto more accurately describe the behavior being tested.
🧹 Nitpick comments (5)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (4)
311-321: DisposeJsonDocumentto release pooled memory.
JsonDocumentimplementsIDisposableand uses pooled arrays internally. Wrap it in ausingstatement to ensure proper cleanup.var content = await response.Content.ReadAsStringAsync(); - var json = JsonDocument.Parse(content); - if (json.RootElement.TryGetProperty("count", out var countProp)) + using var json = JsonDocument.Parse(content); + if (json.RootElement.TryGetProperty("count", out var countProp)) {
474-479: DisposeJsonDocumentinstances in query blocks.Both database query blocks parse JSON without disposing the
JsonDocument. Addusingto prevent memory pressure from pooled arrays.var content1 = await response1.Content.ReadAsStringAsync(); - var json1 = JsonDocument.Parse(content1); + using var json1 = JsonDocument.Parse(content1); if (json1.RootElement.TryGetProperty("count", out var countProp1))Apply the same fix to the second query block (line 494):
var content2 = await response2.Content.ReadAsStringAsync(); - var json2 = JsonDocument.Parse(content2); + using var json2 = JsonDocument.Parse(content2); if (json2.RootElement.TryGetProperty("count", out var countProp2))
566-577: DisposeJsonDocumentin helper method.Same issue as the test methods - wrap in
usingstatement.var content = await response.Content.ReadAsStringAsync(); - var json = JsonDocument.Parse(content); + using var json = JsonDocument.Parse(content); if ( json.RootElement.TryGetProperty("count", out var count))
579-582: Consider logging swallowed exceptions for debugging flaky tests.The empty catch block is acceptable for retry logic, but silently swallowing exceptions can make diagnosing test failures difficult. Consider logging the exception type/message.
catch { - // Retry + // Retry - log for debugging flaky tests + TestContext.WriteLine($"Retry {attempts + 1}/{maxAttempts}: Query failed"); }src/net-questdb-client-tests/MultiUrlHttpTests.cs (1)
376-419: Reflection-based test is fragile; consider alternatives.This test relies on private implementation details (
_clientCache,_handlerCache,CleanupUnusedClients) via reflection. If the implementation changes field names or types, this test will silently pass or fail for the wrong reasons.Consider:
- Exposing a
VisibleForTestinginternal method if this behavior is critical- Testing this indirectly through public API behavior (e.g., verify no exception during repeated rotation + cleanup cycles)
- Adding a code comment explaining this is intentionally testing internal behavior for regression prevention
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/net-questdb-client-tests/MultiUrlHttpTests.cs(1 hunks)src/net-questdb-client-tests/QuestDbIntegrationTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/net-questdb-client-tests/MultiUrlHttpTests.cs (1)
src/net-questdb-client/Utils/AddressProvider.cs (3)
AddressProvider(30-166)AddressProvider(39-48)RotateToNextAddress(89-92)
🔇 Additional comments (14)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (4)
38-53: LGTM!The OneTimeSetUp/OneTimeTearDown pattern correctly manages the QuestDB lifecycle for the test fixture. The null check and sequential disposal is appropriate.
59-154: LGTM!These basic integration tests follow a clean pattern and appropriately handle eventual consistency with retry logic where needed.
156-177: LGTM!The multi-URL fallback test correctly validates failover behavior by placing a non-existent endpoint first in the address list.
179-198: LGTM!The auto-flush test validates the automatic flushing behavior with a single-row threshold.
src/net-questdb-client-tests/MultiUrlHttpTests.cs (10)
76-104: LGTM!Test correctly verifies that data is sent to the first available address and the second server receives nothing. Good use of
usingstatements for resource cleanup.
106-132: LGTM!Failover test is well-structured - simulates a retriable error on the first server and verifies the second server receives the data.
177-193: LGTM!Properly tests the error path when all configured servers are unavailable. The
retry_timeout=1000keeps the test from running too long.
195-253: LGTM!Good coverage of both sequential address usage and synchronous send path. Both tests properly verify data arrives at expected servers.
255-282: LGTM!Correctly verifies that no rotation occurs when the first address succeeds, and that the second server remains unused.
284-310: LGTM!Clean unit test for AddressProvider rotation logic. Covers initial state, sequential rotation, and wrap-around behavior.
330-359: LGTM!Excellent coverage of IPv6 edge cases including loopback, full addresses, addresses with many colons, and addresses without ports. The
-1return for missing port (line 353) is a sensible sentinel value.
361-374: LGTM!Properly verifies that single-address rotation returns the same address and
HasMultipleAddressesis false.
415-418: No action needed. HttpSender.Dispose() disposes all cached clients and handlers regardless of their origin, including externally-injected test entries. No resource leak occurs.Likely an incorrect or invalid review comment.
312-328: IPv6 parsing implementation is correct and matches test expectations.The
ParseHostimplementation correctly detects addresses starting with[, extracts the closing bracket usingIndexOf(']'), and returns the full bracketed section viaSubstring(0, closingBracketIndex + 1). For the test case[::1]:9000, this returns[::1]as expected. TheParsePortimplementation similarly handles IPv6 by checking for a colon immediately after the closing bracket and extracting the port portion, which correctly parses9000from[::1]:9000. The implementation satisfies all test expectations.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/net-questdb-client-tests/QuestDbManager.cs (4)
30-30: Modernize string slicing and specify Guid format.Line 30 uses the older
Substringmethod and relies on Guid's default format. Consider using the range operator[..8]and an explicit format specifier for consistency:- _containerName = $"{ContainerNamePrefix}{port}-{httpPort}-{Guid.NewGuid().ToString().Substring(0, 8)}"; + _containerName = $"{ContainerNamePrefix}{port}-{httpPort}-{Guid.NewGuid().ToString("N")[..8]}";
55-58: Consider validating that the container hasn't started yet.Setting a volume after the container has already started would have no effect. To prevent incorrect usage, consider adding a guard:
public void SetVolume(string volumeName) { + if (IsRunning) + { + throw new InvalidOperationException("Cannot set volume after container has started"); + } _volumeName = volumeName; }
244-277: UseStartsWithfor more precise container name matching.Line 264 uses
Containsto check for the container prefix, but since valid test containers always start withContainerNamePrefix, usingStartsWithwould be more precise and prevent potential false positives:- if (name.Contains(ContainerNamePrefix, StringComparison.Ordinal) && + if (name.StartsWith(ContainerNamePrefix, StringComparison.Ordinal) && (name.Contains($"-{_port}-{_httpPort}-", StringComparison.Ordinal) || name.Contains($"-{_httpPort}-{_port}-", StringComparison.Ordinal)))
279-305: Dispose of the Process instance to release resources.The
Processobject implementsIDisposablebut is never disposed. While the garbage collector will eventually clean it up, explicit disposal is better practice for test infrastructure that may create many processes:- var process = Process.Start(startInfo); + using var process = Process.Start(startInfo); if (process == null) { throw new InvalidOperationException("Failed to start docker command"); }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/net-questdb-client-tests/QuestDbManager.cs (3)
30-30: Simplify GUID substring extraction.The explicit
.ToString()call is unnecessary in string interpolation, and.Substring(0, 8)can be replaced with indexing for slightly better readability.Apply this diff:
- _containerName = $"{ContainerNamePrefix}{port}-{httpPort}-{Guid.NewGuid().ToString().Substring(0, 8)}"; + _containerName = $"{ContainerNamePrefix}{port}-{httpPort}-{Guid.NewGuid():N}"[..^24];Or alternatively:
- _containerName = $"{ContainerNamePrefix}{port}-{httpPort}-{Guid.NewGuid().ToString().Substring(0, 8)}"; + var guidPart = Guid.NewGuid().ToString()[..8]; + _containerName = $"{ContainerNamePrefix}{port}-{httpPort}-{guidPart}";
49-49: Unnecessary null-conditional operator.The
_httpClientfield is always initialized in the constructor and never set to null, so the?.operator is unnecessary.Apply this diff:
- _httpClient?.Dispose(); + _httpClient.Dispose();
123-170: Consider cleanup if StartAsync fails after container creation.If
WaitForQuestDbAsync()throws or any exception occurs after line 163, the container will remain running butIsRunningstays false. Subsequent calls toStartAsync()orDisposeAsync()won't clean it up automatically. Consider wrapping the startup in a try-catch to ensure cleanup on failure, or rely onCleanupExistingContainersAsync()to handle orphaned containers on the next run.Consider adding error handling:
_containerId = output.Trim(); Console.WriteLine($"QuestDB container started: {_containerId}"); - - // Wait for QuestDB to be ready - await WaitForQuestDbAsync(); - IsRunning = true; + try + { + // Wait for QuestDB to be ready + await WaitForQuestDbAsync(); + IsRunning = true; + } + catch + { + // Clean up the container if startup fails + await RunDockerCommandAsync($"rm -f {_containerName}"); + _containerId = null; + throw; + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs(1 hunks)src/net-questdb-client-tests/QuestDbManager.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/net-questdb-client-tests/QuestDbManager.cs (2)
src/net-questdb-client/Senders/HttpSender.cs (7)
HttpClient(219-264)HttpClient(269-282)TimeSpan(375-379)Dispose(824-843)Task(421-438)Task(651-665)Task(668-791)src/net-questdb-client-tests/QuestDbIntegrationTests.cs (2)
Task(541-545)Task(547-590)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (2)
src/net-questdb-client/Senders/HttpSender.cs (6)
Task(421-438)Task(651-665)Task(668-791)HttpClient(219-264)HttpClient(269-282)TimeSpan(375-379)src/net-questdb-client/Sender.cs (1)
Sender(48-96)
🔇 Additional comments (9)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (9)
38-53: LGTM!The fixture setup and teardown correctly manage the QuestDB lifecycle with proper async disposal and null checking.
59-75: LGTM!The HTTP ingestion test is straightforward and properly structured with resource disposal.
77-93: LGTM!The ILP ingestion test correctly validates TCP protocol connectivity and data ingestion.
95-116: LGTM!The multiple rows test correctly batches rows and uses
GreaterThanOrEqualToto account for potential timing-related duplicates.
118-154: LGTM!The data types test validates multiple column types with appropriate retry logic for eventual consistency.
156-177: LGTM!The multi-URL fallback test correctly validates that the client can recover from unavailable endpoints.
179-198: LGTM!The auto-flush test validates the automatic flushing behavior. The 100ms delay should be sufficient for most environments, and the verification helper includes its own retry logic as a safety net.
200-345: LGTM!The database restart chaos test comprehensively validates resilience during restarts. The retry logic, concurrent task orchestration, and assertion with
GreaterThanOrEqualToare all appropriate for this scenario.
541-590: LGTM!The helper methods correctly handle data verification with proper retry logic and HTTP client disposal. The URL construction at line 562 correctly includes the
http://scheme.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (3)
208-208: Consider using range operator for string slicing.The
Substring(0, 8)works correctly but C# 8+ supports the more concise range operator syntax[..8].Apply this diff:
- var volumeName = $"questdb-test-vol-{Guid.NewGuid().ToString().Substring(0, 8)}"; + var volumeName = $"questdb-test-vol-{Guid.NewGuid().ToString()[..8]}";
355-356: Consider using range operator for string slicing.Same as line 208, the
Substring(0, 8)can be modernized to use the range operator[..8].Apply this diff:
- var volume1 = $"questdb-test-vol-db1-{Guid.NewGuid().ToString().Substring(0, 8)}"; - var volume2 = $"questdb-test-vol-db2-{Guid.NewGuid().ToString().Substring(0, 8)}"; + var volume1 = $"questdb-test-vol-db1-{Guid.NewGuid().ToString()[..8]}"; + var volume2 = $"questdb-test-vol-db2-{Guid.NewGuid().ToString()[..8]}";
579-582: Consider logging exceptions in the catch block.The empty catch block silently swallows exceptions during retries, which can make debugging test failures more difficult. Consider at least logging the exception to TestContext.
Apply this diff:
} - catch + catch (Exception ex) { - // Retry + // Retry on transient errors + TestContext.WriteLine($"Query attempt {attempts + 1} failed: {ex.Message}"); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs(1 hunks)
🔇 Additional comments (6)
src/net-questdb-client-tests/QuestDbIntegrationTests.cs (6)
35-58: LGTM!The test fixture setup and teardown logic is correct, with proper null checking and async lifecycle management.
59-177: LGTM!The basic integration tests are well-structured and correctly validate HTTP/ILP protocols, multiple rows, various data types, and multi-URL failover scenarios.
179-198: LGTM!The auto-flush test correctly validates that data is automatically flushed when
auto_flush_rows=1is configured.
210-296: LGTM!The chaos test correctly simulates database restarts during active writes, validating the client's retry and resilience capabilities.
331-345: LGTM!The assertion and cleanup logic are correct, with proper resource disposal in the finally block.
376-520: LGTM!The multi-database failover test correctly orchestrates database restarts and validates that data is written across both instances with proper error handling and retry logic.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @ideoma. * #59 (comment) The following files were modified: * `src/dummy-http-server/DummyHttpServer.cs` * `src/dummy-http-server/IlpEndpoint.cs` * `src/net-questdb-client-tests/QuestDbManager.cs` * `src/net-questdb-client/Buffers/BufferV1.cs` * `src/net-questdb-client/Buffers/IBuffer.cs` * `src/net-questdb-client/Senders/AbstractSender.cs` * `src/net-questdb-client/Senders/HttpSender.cs` * `src/net-questdb-client/Senders/ISender.cs` * `src/net-questdb-client/Utils/AddressProvider.cs` * `src/net-questdb-client/Utils/SenderOptions.cs`
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci/azurre-binaries-pipeline.yml (1)
54-59: Fix the version mismatch in .NET 10.0 SDK installation.The display name indicates this task should install .NET 10.0 SDK, but the version references
$(dotnetVersion)which is defined as'9.0.x'at line 9. This will install .NET 9.0 SDK instead of 10.0, breaking multi-targeting support.Apply this diff to fix the version:
- task: UseDotNet@2 displayName: 'Install .NET 10.0 SDK' inputs: packageType: 'sdk' - version: $(dotnetVersion) + version: '10.0.x' installationPath: $(Agent.ToolsDirectory)/dotnet
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ci/azurre-binaries-pipeline.yml(1 hunks)
🔇 Additional comments (1)
ci/azurre-binaries-pipeline.yml (1)
47-52: LGTM!The explicit inputs block for the .NET 9.0 SDK installation is correctly formatted.
This PR adds support for configuring multiple urls in the .NET client. In the event that one of the instances is unavailable for writes, the client will retry against alternate urls.
There's also some small API changes to add GUID and char support.
Also closes: #50
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.